[wrangler|topotools] Migrate UpdateShardRecords, RefreshTabletsByShard, and {Get,Save}RoutingRules#7965
Conversation
…}RoutingRules` I am moving these to `package topotools` to reuse this code in both `package grpcvtctldserver` (which wrangler imports) and `package wrangler`. Signed-off-by: Andrew Mason <amason@slack-corp.com>
rafael
left a comment
rafael
left a comment
There was a problem hiding this comment.
This looks good. Left a small comment. Let me know what you think.
go/vt/topotools/keyspace.go
Outdated
| // For 'to' shards, refresh to make them serve. The 'from' shards will | ||
| // be refreshed after traffic has migrated. | ||
| if !isFrom { | ||
| _ = RefreshTabletsByShard(ctx, ts, tmc, si, cells, logger) |
There was a problem hiding this comment.
I like this pattern of being explicit that we are ignoring the error. However, is not a pattern that I've seen in the codebase.
I wonder if start adding here will create confusion.
There was a problem hiding this comment.
My opinion is being that explicit about ignoring the error is a better (or at least more clear) pattern, and we've got to start somewhere! 😂 I'll do a quick temp check to see what some other folks think about this
There was a problem hiding this comment.
That sounds like a plan. If we can get thumbs-up that we would like to make this a standard pattern moving forward, let's start here!
There was a problem hiding this comment.
Consensus seemed to be:
- yes, explicitly acknowledging that we're ignoring errors is better than implicitly doing so
- we've had lots of trouble debugging tests specifically due to errors being discarded
- we should log the ignored error in this specific case, and whether to log or not is probably a case-by-case basis
- i'm also going to work on adding
errcheckto the linter so we can start to enforce this as a pattern going forward
There was a problem hiding this comment.
Great, thanks for following up on this. All makes sense.
Signed-off-by: Andrew Mason <amason@slack-corp.com>
ajm188
added
Type: Internal Cleanup
Component: Cluster management
and removed
Component: Build/CI
Type: CI/Build
labels
…funcs_to_topotools
[wrangler|topotools] Migrate `UpdateShardRecords`, `RefreshTabletsByShard`, and `{Get,Save}RoutingRules`
…funcs_to_topotools
[wrangler|topotools] Migrate `UpdateShardRecords`, `RefreshTabletsByShard`, and `{Get,Save}RoutingRules`
Signed-off-by: Vitaliy Mogilevskiy <vmogilevskiy@slack-corp.com>
3 participants
Description
I am moving these to
package topotoolsto reuse this code in bothpackage grpcvtctldserver(which wrangler imports) andpackage wrangler.Signed-off-by: Andrew Mason amason@slack-corp.com
Related Issue(s)
Checklist
Deployment Notes